Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix rosie REST API docs #2334

Merged
merged 1 commit into from
May 10, 2019
Merged

fix rosie REST API docs #2334

merged 1 commit into from
May 10, 2019

Conversation

oliver-sanders
Copy link
Member

Fix a couple of broken examples in the docs.

@oliver-sanders oliver-sanders added this to the next-feature milestone May 10, 2019
@oliver-sanders oliver-sanders self-assigned this May 10, 2019
matthewrmshin
matthewrmshin reviewed May 10, 2019
View reviewed changes
sphinx/api/rosie-web.rst Outdated
@@ -152,7 +152,7 @@ REST API

.. code-block:: http

GET http://host/my_prefix/search?var+bob+nowcast&format=json HTTP/1.1
GET http://host/my_prefix/search/var+bob+nowcast&format=json HTTP/1.1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't the old syntax work any more?

Copy link
Member Author

@oliver-sanders oliver-sanders May 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this syntax was ever correct, was it?

It's an invalid URL parameter, must be ?key=value[&key2=value2...].

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's true.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably either an error copied forward from the old docs or a translation error by yours truely 😦

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this meant to instead be:

Suggested change
GET http://host/my_prefix/search/var+bob+nowcast&format=json HTTP/1.1
GET http://host/my_prefix/search?s=var+bob+nowcast&format=json HTTP/1.1

The original, as with your change suggestion, both don't make sense as URL queries, but this does? To check I am not completely mistaken, I tried these URLs out on both the current ("old") & my new (PR #2288) versions of the rosie discovery utility, & the original & change here both give a 404 error.

sadielbartholomew
sadielbartholomew suggested changes May 10, 2019
View reviewed changes
Copy link
Contributor

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one of these amendments needs changing.

sphinx/api/rosie-web.rst Outdated
@@ -152,7 +152,7 @@ REST API

.. code-block:: http

GET http://host/my_prefix/search?var+bob+nowcast&format=json HTTP/1.1
GET http://host/my_prefix/search/var+bob+nowcast&format=json HTTP/1.1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this meant to instead be:

Suggested change
GET http://host/my_prefix/search/var+bob+nowcast&format=json HTTP/1.1
GET http://host/my_prefix/search?s=var+bob+nowcast&format=json HTTP/1.1

The original, as with your change suggestion, both don't make sense as URL queries, but this does? To check I am not completely mistaken, I tried these URLs out on both the current ("old") & my new (PR #2288) versions of the rosie discovery utility, & the original & change here both give a 404 error.

@oliver-sanders
Copy link
Member Author

The original & change here both give a 404 error

The change doesn't give a 404, for example:

http://fcm1/rosie/mot/search/frsn

However this isn't the user facing page not the REST API! Whoops! s is what we were supposed to document here:

http://fcm1/rosie/mot/search?s=frsn

sadielbartholomew
sadielbartholomew reviewed May 10, 2019
View reviewed changes
Copy link
Contributor

@sadielbartholomew sadielbartholomew left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! s is what we were supposed to document here

There appears to be the same issue with the query endpoint (see line 94: :param list query: List of queries) which following that logic should be :param list q:

@oliver-sanders
Copy link
Member Author

Yep, that should be the last of them.

sadielbartholomew
sadielbartholomew approved these changes May 10, 2019
View reviewed changes
Copy link
Contributor

@sadielbartholomew sadielbartholomew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally we could have search?s= rather than search/?s=, at the very least for consistency with the query endpoint URL documented above it but also as it is the strict endpoint, but since it gives the same result & I am pedantic, it is not a blocker.

@sadielbartholomew sadielbartholomew merged commit d1eee0e into metomi:master May 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewrmshin matthewrmshin matthewrmshin approved these changes

@sadielbartholomew sadielbartholomew sadielbartholomew approved these changes

Assignees

@oliver-sanders oliver-sanders

Labels
doc
Projects
None yet
Milestone
2.0a1
Development

Successfully merging this pull request may close these issues.
3 participants
@oliver-sanders @matthewrmshin @sadielbartholomew